-
Notifications
You must be signed in to change notification settings - Fork 81
Add image optimization with Cloudflare Images #999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add image optimization with Cloudflare Images #999
Conversation
|
| status: 508, | ||
| }); | ||
| } | ||
| throw new Error("Failed to fetch image"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is consistent with Next.js but not sure if a 500 response is the best if the URL is invalid
|
Hello @pilcrowonpaper, good to see you here :) looks good on first glance, I'll do another review later! |
commit: |
| } else { | ||
| const cacheControlHeader = imageResponse.headers.get("Cache-Control"); | ||
| if (cacheControlHeader !== null) { | ||
| immutable = cacheControlHeader.includes("immutable"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next.js does something more complicated and bases the optimized response on the upstream Cache-Control header. I didn't want to do anything complex for now so it just checks if the upstream image is a static content
|
It looks like the Playwright tests are failing because the test cases don't include the required |
| const __IMAGES_CONTENT_DISPOSITION__ = JSON.stringify( | ||
| imagesManifest?.images?.contentDispositionType ?? "attachment" | ||
| ); | ||
| const __IMAGES_MAX_REDIRECTS__ = JSON.stringify(imagesManifest?.images?.maximumRedirects ?? 3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was changed to 3 from unlimited in Next.js 16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use unlimited then (and maybe add your comment as a code comment on this file/line)
|
|
||
| const defaultDeviceSizes = [640, 750, 828, 1080, 1200, 1920, 2048, 3840]; | ||
| const defaultImageSizes = [32, 48, 64, 96, 128, 256, 384]; | ||
| const defaultQualities = [75]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before Next.js 16, the default behavior was to allow values from 1-100
| } | ||
|
|
||
| const defaultDeviceSizes = [640, 750, 828, 1080, 1200, 1920, 2048, 3840]; | ||
| const defaultImageSizes = [32, 48, 64, 96, 128, 256, 384]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before Next.js 16, 16 was also included
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use Next 16 (and add comments on the lines).
nit: maybe move those constants up (i.e. before they are used)
| async function fetchImage(url: string, count: number): Promise<FetchImageResult> { | ||
| let response: Response; | ||
| try { | ||
| response = await fetch(url, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't added support for dangerouslyAllowLocalIP, which checks if a domain resolves to a private IP.
| ) { | ||
| const imageUrl = url.searchParams.get("url") ?? ""; | ||
| return await fetchImage(env.ASSETS, imageUrl, ctx); | ||
| return await handleImageRequest(url, request.headers, env); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe pass the request instead of url and headers to handleImageRequest ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already create a URL instance in the worker so this allows us to reuse it and avoid parsing the URL string twice
|
@vicb |
| }; | ||
|
|
||
| let NEXT_IMAGE_REGEXP: RegExp; | ||
| export async function handleImageRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add JSDoc to this function
| const result: ErrorResult = { | ||
| ok: false, | ||
| message: '"url" parameter is required', | ||
| }; | ||
| return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: That should be enough?
| const result: ErrorResult = { | |
| ok: false, | |
| message: '"url" parameter is required', | |
| }; | |
| return result; | |
| return { | |
| ok: false, | |
| message: '"url" parameter is required', | |
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I don't like returning objects directly and prefer explicitly defining the type. I can change it if it bothers you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use satisfies if you really want to add a type.
But all "nit" comments are mostly suggestion, pick what you prefer.
| url = urlQueryValue; | ||
|
|
||
| const pathname = getPathnameFromRelativeURL(url); | ||
| if (/\/_next\/image($|\/)/.test(decodeURIComponent(pathname))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about /_next/image?url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't understand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would not match as you have a $
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It matches against the pathname so it should be fine, no? It matches against /_next/image, /_next/image/, and /_next/image/*
| : {}; | ||
|
|
||
| const __IMAGES_REMOTE_PATTERNS__ = JSON.stringify(imagesManifest?.images?.remotePatterns ?? []); | ||
| const __IMAGES_LOCAL_PATTERNS_DEFINED__ = JSON.stringify( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed given that __IMAGES_LOCAL_PATTERNS__ default to an empty list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Next.js treats undefined and an empty list differently
| return result; | ||
| } | ||
| const widthQueryValue = widthQueryValues[0]!; | ||
| if (!/^[0-9]+$/.test(widthQueryValue)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
| if (!/^[0-9]+$/.test(widthQueryValue)) { | |
| if (!/^\d+$/.test(widthQueryValue)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context, most of the parameter validation is based on Next.js' own code
| if (!hasRemoteMatch(remotePatterns, parsedURL)) { | ||
| const result: ErrorResult = { | ||
| ok: false, | ||
| message: '"url" parameter is not allowed', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe you can drop ok and rename "message" to "error"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't like dealing with undefined and optional parameters so I prefer having with an ok property that is always defined, but let me know if you want me to change it
| url: url, | ||
| width: width, | ||
| quality: quality, | ||
| format: format, | ||
| static: staticAsset, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| url: url, | |
| width: width, | |
| quality: quality, | |
| format: format, | |
| static: staticAsset, | |
| url, | |
| width, | |
| quality, | |
| format, | |
| static: staticAsset, |
| return new Response('"url" parameter is valid but upstream response is invalid', { | ||
| status: 400, | ||
| }); | ||
| type ParseImageRequestURLResult = ParseImageRequestURLSuccessResult | ErrorResult; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: move the type above the function where they are used
| const WEBP = "image/webp"; | ||
| const PNG = "image/png"; | ||
| const JPEG = "image/jpeg"; | ||
| const JXL = "image/jxl"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: why have those been removed? (IIRC it comes from Next)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed formats that aren't supported by Cloudflare:
https://developers.cloudflare.com/images/transform-images/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the image binding is not used and we try to access one of this removed type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That part I was unsure. I personally think we should only serve images that are supported by the optimization process, even when the binding isn't defined. It'd be weird if your images stopped being served when you turn on image optimization by defining the binding. It will be a breaking change though
| let imageResponse: Response; | ||
| if (parseResult.url.startsWith("/")) { | ||
| if (env.ASSETS === undefined) { | ||
| console.error("env.ASSETS binding is not defined."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the logger rather than console.
Also we should fallback to the former behavior (serve unoptimized images) when the binding is not present)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops that's ASSET, nvm the comment about former behavior but still please switch to using the logger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had some issues with wrangler not being able resolve @opennextjs/aws imports but I'll look into it again
| const imageTransformationResult = await imageSource | ||
| .transform({ | ||
| width: parseResult.width, | ||
| fit: "scale-down", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Does not Next always use scale-down?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure. There's so no documentation on the default value
https://developers.cloudflare.com/images/transform-images/transform-via-url/#fit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the typo in my initial message, the question was about Next
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll need to double check, but I just assumed so because you mentioned that the images shouldn't be scaled more than the original size. (For context, scale-down will only scale down and never scale up images.)
vicb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome @pilcrowonpaper !
I have done a frist round of review.
Could you please add JSDoc comments to added functions.
Maybe there could be separate functions to i.e. validate each parameter?
No description provided.